Skip to content

fix: handle unparameterized interval_day type#178

Closed
thgschwind wants to merge 1 commit into
substrait-io:mainfrom
thgschwind:working
Closed

fix: handle unparameterized interval_day type#178
thgschwind wants to merge 1 commit into
substrait-io:mainfrom
thgschwind:working

Conversation

@thgschwind

Copy link
Copy Markdown
Contributor

Allow interval_day to be used without an explicit precision parameter.

Fixes: #177

Allow interval_day to be used without an explicit precision parameter.

Signed-off-by: Thomas Gschwind <thg@zurich.ibm.com>
Comment on lines +154 to +159
if parametrized_type.precision is None:
return Type(
interval_day=Type.IntervalDay(
nullability=nullability,
)
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a little bit more context on whether this is only an issue for IntervalDay for you and how this is triggered? I'm asking since we have very similar code above for PrecisionTimestampContext and PrecisionTimestampTZContext.

@nielspardon

Copy link
Copy Markdown
Member

Thanks for putting this together, @thgschwind, and apologies for the slow turnaround.

Revisiting this in light of the substrait v0.97.0 spec change (July 12): the premise that interval_day may be used without a precision no longer holds, so I don't think we can take this approach.

  • Proto: v0.97.0 changed IntervalDay.precision's contract to "Implementations should reject an unset precision" (previously an unset precision was treated as 6/microseconds). This PR does the opposite — it produces a Type.IntervalDay with unset precision, i.e. exactly the message the spec now says to reject.
  • Grammar: interval_day has no bare form in SubstraitType.g4 — the only alternative is interval_day<precision>. evaluate("interval_day") is actually a syntax error (mismatched input '<EOF>' expecting {'<','?'}); it only reaches the precision is None branch because ANTLR error recovery hands back a partial parse tree.

Revisiting my earlier question about whether this is IntervalDay-specific — it isn't. Every precision-parameterized type (decimal, interval_day, interval_compound, precision_time, precision_timestamp, precision_timestamp_tz) requires explicit parameters, so bare forms are all invalid in the same way.

Closing without merging, since defaulting to unset precision is now spec-noncompliant. The one real remaining problem — the confusing Unknown token type NoneType on invalid syntax — is worth fixing on its own and I've split it out into #216.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

interval_day type without precision parameter causes error in derivation expression evaluation

2 participants